Skip to content

Conversation

@msutkowski
Copy link
Contributor

Description

Expands on #957. Allows order and data to be correctly typed when there are nested collections/schemas. I can merge this into that branch once I get feedback on the below. Here is a TS playground showing the type resolution:

Example usage:

export interface FirestoreTask {
  target: string;
  from: string;
  direction: "inbound" | "outbound";
}

interface FirestoreSchema {
  organizations: {
    tasks: FirestoreReducer.Entity<FirestoreTask>;
  };
}

// rootReducer.ts
combineReducers({
    firestore: firestoreReducer as Reducer<
      FirestoreReducer.Reducer<FirestoreSchema>
    >,
   ... rest
  });
export type RootState = ReturnType<typeof rootReducer>;

// selector.ts
export const selectFirestoreTasks = (state: RootState, key: string) =>
  Object.values(state.firestore.data.organizations?.[key]?.tasks || {});

Questions:
Should this work even be done on this repo? It seems like there is some duplication between this repo and redux-firestore.

Todo:

  • Create a createTypedFirestoreReducer to make this easier.
  • Update the redux-firestore package as well?
  • Update docs with new examples

Check List

If not relevant to pull request, check off as complete

  • All tests passing
  • Docs updated with any changes or examples if applicable
  • Added tests to ensure new feature(s) work properly

Relevant Issues

@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #964 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #964   +/-   ##
=======================================
  Coverage   88.33%   88.33%           
=======================================
  Files          29       29           
  Lines         797      797           
=======================================
  Hits          704      704           
  Misses         93       93           

@msutkowski
Copy link
Contributor Author

Adding a couple of notes here: The example above was using nested data, which obviously goes against the recommendation of the docs. The good news is this typing approach is still valid when flattened. So - we'd be covered in the event folks have bad schemas (like me!) or follow the docs.

export const selectFirestoreTasks = (state: RootState, subdomain: string) =>
  state.firestore.ordered.tasks || [];

interface FirestoreSchema {
  tasks: FirestoreReducer.Entity<FirestoreTask>;
}

It was also recommended to do something like this on firestoreReducer so that TS users can have it typed easily:

combineReducers({ firestore: firestoreReducer.forSchema<Schema>() })

My only other thought regarding the reducer is that if we're passing in a schema, we should also use it to initialize the firestore reducer state to get rid of the extra initializer like in the selectFirestoreTasks selector above.

@msutkowski
Copy link
Contributor Author

Closing this out and will open against the typescript branch of redux-firestore if desired.

@msutkowski msutkowski closed this Jun 16, 2020
prescottprue added a commit to prescottprue/redux-firestore that referenced this pull request Feb 15, 2021
* fix(getFirestore): correctly load extended firestore instance in `getFirestore` (#318) - @aarajh 
* fix(orderedReducer): reorder items when `newIndex` is zero (#317) - @matthew-h-cromer
* feat(types): add types for reducer (#296, [react-redux-firebase 964](prescottprue/react-redux-firebase#964)) - @msutkowski,@bawahakim
* fix(examples): update complete example patterns + major versions
* build(deps): bump immer from 5.0.0 to 8.0.1 (#320)
* build(deps): bump ini from 1.3.5 to 1.3.8 (#316)
* build(deps): bump ini from 1.3.5 to 1.3.8 in /examples/basic (#315)

Co-Authored-By: Hakim Bawa <[email protected]>
Co-authored-by: Aaraj Habib <[email protected]>
Co-authored-by: matthew-h-cromer <[email protected]>
Co-authored-by: Matt Sutkowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant